Skip to content

Conversation

@jansvoboda11
Copy link
Contributor

This PR uses the new-ish llvm::vfs::FileSystem::visit() interface to collect VFS overlay entries from an existing FileSystem instance rather than parsing the VFS YAML file anew. This prevents duplicate diagnostics as observed by clang/test/VFS/broken-vfs-module-dep.c.

@llvmbot llvmbot added clang Clang issues not falling into any other category llvm:support labels Sep 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 12, 2025

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

This PR uses the new-ish llvm::vfs::FileSystem::visit() interface to collect VFS overlay entries from an existing FileSystem instance rather than parsing the VFS YAML file anew. This prevents duplicate diagnostics as observed by clang/test/VFS/broken-vfs-module-dep.c.


Full diff: https://github.com/llvm/llvm-project/pull/158372.diff

4 Files Affected:

  • (modified) clang/lib/Frontend/CompilerInstance.cpp (+4-11)
  • (modified) clang/test/VFS/broken-vfs-module-dep.c (-1)
  • (modified) llvm/include/llvm/Support/VirtualFileSystem.h (+3-7)
  • (modified) llvm/lib/Support/VirtualFileSystem.cpp (+3-13)
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 31a8d75fec4bd..3e67cf157cb1c 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -271,19 +271,12 @@ static void collectIncludePCH(CompilerInstance &CI,
 
 static void collectVFSEntries(CompilerInstance &CI,
                               std::shared_ptr<ModuleDependencyCollector> MDC) {
-  if (CI.getHeaderSearchOpts().VFSOverlayFiles.empty())
-    return;
-
   // Collect all VFS found.
   SmallVector<llvm::vfs::YAMLVFSEntry, 16> VFSEntries;
-  for (const std::string &VFSFile : CI.getHeaderSearchOpts().VFSOverlayFiles) {
-    llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> Buffer =
-        llvm::MemoryBuffer::getFile(VFSFile);
-    if (!Buffer)
-      return;
-    llvm::vfs::collectVFSFromYAML(std::move(Buffer.get()),
-                                  /*DiagHandler*/ nullptr, VFSFile, VFSEntries);
-  }
+  CI.getVirtualFileSystem().visit([&](llvm::vfs::FileSystem &VFS) {
+    if (auto *RedirectingVFS = dyn_cast<llvm::vfs::RedirectingFileSystem>(&VFS))
+      llvm::vfs::collectVFSEntries(*RedirectingVFS, VFSEntries);
+  });
 
   for (auto &E : VFSEntries)
     MDC->addFile(E.VPath, E.RPath);
diff --git a/clang/test/VFS/broken-vfs-module-dep.c b/clang/test/VFS/broken-vfs-module-dep.c
index 2336306de8c6d..1c371a13e85c9 100644
--- a/clang/test/VFS/broken-vfs-module-dep.c
+++ b/clang/test/VFS/broken-vfs-module-dep.c
@@ -2,6 +2,5 @@
 // RUN: mkdir -p %t
 // RUN: not %clang_cc1 -module-dependency-dir %t -ivfsoverlay %S/Inputs/invalid-yaml.yaml %s 2>&1 | FileCheck %s
 
-// CHECK: error: Unexpected token
 // CHECK: error: Unexpected token
 // CHECK: 1 error generated
diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h
index d97677305a39f..8007f3c853f20 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -1114,14 +1114,10 @@ class LLVM_ABI RedirectingFileSystem
 };
 
 /// Collect all pairs of <virtual path, real path> entries from the
-/// \p YAMLFilePath. This is used by the module dependency collector to forward
+/// \p VFS. This is used by the module dependency collector to forward
 /// the entries into the reproducer output VFS YAML file.
-LLVM_ABI void collectVFSFromYAML(
-    std::unique_ptr<llvm::MemoryBuffer> Buffer,
-    llvm::SourceMgr::DiagHandlerTy DiagHandler, StringRef YAMLFilePath,
-    SmallVectorImpl<YAMLVFSEntry> &CollectedEntries,
-    void *DiagContext = nullptr,
-    IntrusiveRefCntPtr<FileSystem> ExternalFS = getRealFileSystem());
+void collectVFSEntries(RedirectingFileSystem &VFS,
+                       SmallVectorImpl<YAMLVFSEntry> &CollectedEntries);
 
 class YAMLVFSWriter {
   std::vector<YAMLVFSEntry> Mappings;
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp
index 5d4248819f1fb..cf784595c2f1c 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -2707,19 +2707,9 @@ static void getVFSEntries(RedirectingFileSystem::Entry *SrcE,
   Entries.push_back(YAMLVFSEntry(VPath.c_str(), FE->getExternalContentsPath()));
 }
 
-void vfs::collectVFSFromYAML(std::unique_ptr<MemoryBuffer> Buffer,
-                             SourceMgr::DiagHandlerTy DiagHandler,
-                             StringRef YAMLFilePath,
-                             SmallVectorImpl<YAMLVFSEntry> &CollectedEntries,
-                             void *DiagContext,
-                             IntrusiveRefCntPtr<FileSystem> ExternalFS) {
-  std::unique_ptr<RedirectingFileSystem> VFS = RedirectingFileSystem::create(
-      std::move(Buffer), DiagHandler, YAMLFilePath, DiagContext,
-      std::move(ExternalFS));
-  if (!VFS)
-    return;
-  ErrorOr<RedirectingFileSystem::LookupResult> RootResult =
-      VFS->lookupPath("/");
+void vfs::collectVFSEntries(RedirectingFileSystem &VFS,
+                            SmallVectorImpl<YAMLVFSEntry> &CollectedEntries) {
+  ErrorOr<RedirectingFileSystem::LookupResult> RootResult = VFS.lookupPath("/");
   if (!RootResult)
     return;
   SmallVector<StringRef, 8> Components;

@jansvoboda11 jansvoboda11 merged commit 4957c47 into llvm:main Sep 15, 2025
12 checks passed
@jansvoboda11 jansvoboda11 deleted the vfs-overlay-reparsing branch September 15, 2025 16:37
qiongsiwu pushed a commit to qiongsiwu/llvm-project that referenced this pull request Oct 14, 2025
…lvm#158372)

This PR uses the new-ish `llvm::vfs::FileSystem::visit()` interface to
collect VFS overlay entries from an existing `FileSystem` instance
rather than parsing the VFS YAML file anew. This prevents duplicate
diagnostics as observed by `clang/test/VFS/broken-vfs-module-dep.c`.

(cherry picked from commit 4957c47)
jansvoboda11 added a commit to swiftlang/llvm-project that referenced this pull request Oct 15, 2025
…lvm#158372)

This PR uses the new-ish `llvm::vfs::FileSystem::visit()` interface to
collect VFS overlay entries from an existing `FileSystem` instance
rather than parsing the VFS YAML file anew. This prevents duplicate
diagnostics as observed by `clang/test/VFS/broken-vfs-module-dep.c`.

(cherry picked from commit 4957c47)
jansvoboda11 added a commit to swiftlang/llvm-project that referenced this pull request Oct 18, 2025
…lvm#158372)

This PR uses the new-ish `llvm::vfs::FileSystem::visit()` interface to
collect VFS overlay entries from an existing `FileSystem` instance
rather than parsing the VFS YAML file anew. This prevents duplicate
diagnostics as observed by `clang/test/VFS/broken-vfs-module-dep.c`.

(cherry picked from commit 4957c47)
jansvoboda11 added a commit to swiftlang/llvm-project that referenced this pull request Oct 23, 2025
…lvm#158372)

This PR uses the new-ish `llvm::vfs::FileSystem::visit()` interface to
collect VFS overlay entries from an existing `FileSystem` instance
rather than parsing the VFS YAML file anew. This prevents duplicate
diagnostics as observed by `clang/test/VFS/broken-vfs-module-dep.c`.

(cherry picked from commit 4957c47)
qiongsiwu added a commit to swiftlang/llvm-project that referenced this pull request Oct 27, 2025
…Instance Sharing (#11631)

* [clang][deps] Remove dependency on `tooling::ToolAction` (llvm#149904)

The dependency scanner was initially using a fair amount of
infrastructure provided by the `clangTooling` library. Over time, the
needs for bespoke handling of command lines grew and the overlap with
the tooling library kept shrinking. I don't think the library provides
any value anymore.

I decided to remove the dependency and only reimplement the small bits
required by the scanner.

This allowed for a nice simplification, where we no longer need to
create temporary dummy `FileManager` instances (mis-named as
`DriverFileMgr` in some parts) and `SourceManager` instances to attach
to the `DiagnosticsEngine`. That code was copied from the tooling
library to support `DiagnosticConsumers` that expect these to exist. The
scanner uses a closed set of consumers and none need these objects to
exist.

The motivation for this (hopefully NFC) patch are some new restrictions
to how VFS's can be propagated in Clang that I'm working on.

(cherry picked from commit aa1b416)

* Reland "[clang] Delay normalization of `-fmodules-cache-path` (llvm#150123)"

This reverts commit 613caa9, essentially
reapplying 4a4bdde after moving
`normalizeModuleCachePath` from clangFrontend to clangLex.

This PR is part of an effort to remove file system usage from the
command line parsing code. The reason for that is that it's impossible
to do file system access correctly without a configured VFS, and the VFS
can only be configured after the command line is parsed. I don't want to
intertwine command line parsing and VFS configuration, so I decided to
perform the file system access after the command line is parsed and the
VFS is configured - ideally right before the file system entity is used
for the first time.

This patch delays normalization of the module cache path until
`CompilerInstance` is asked for the cache path in the current
compilation context.

(cherry picked from commit 55bef46)

* NFC: Clean up of IntrusiveRefCntPtr construction from raw pointers. (llvm#151545)

Handles clang::DiagnosticsEngine and clang::DiagnosticIDs.

For DiagnosticIDs, this mostly migrates from `new DiagnosticIDs` to
convenience method `DiagnosticIDs::create()`.

Part of cleanup llvm#151026

(cherry picked from commit c7f3437)

 Conflicts:
	clang/tools/driver/cc1_main.cpp
	clang/unittests/Driver/DXCModeTest.cpp
	clang/unittests/Driver/SimpleDiagnosticConsumer.h
	clang/unittests/Frontend/SearchPathTest.cpp
	clang/unittests/Lex/HeaderSearchTest.cpp
	clang/unittests/Tooling/RewriterTestContext.h

* NFC: Clean up of IntrusiveRefCntPtr construction from raw pointers. (llvm#151782)

This commit handles the following types:
- clang::ExternalASTSource
- clang::TargetInfo
- clang::ASTContext
- clang::SourceManager
- clang::FileManager

Part of cleanup llvm#151026

(cherry picked from commit 4205da0)

 Conflicts:
	clang/lib/Frontend/ASTUnit.cpp
	clang/lib/Frontend/ChainedIncludesSource.cpp
	clang/lib/Frontend/CompilerInstance.cpp

* Merge commit '30633f308941' from llvm.org/main into next

(cherry picked from commit 95ea104)

 Conflicts:
	clang/include/clang/Frontend/CompilerInstance.h
	clang/lib/Frontend/CompilerInstance.cpp

* Merge pull request #11450 from swiftlang/jan_svoboda/cas-fix-early-vfs

[clang] Fix CAS initialization after upstream llvm#158381

(cherry picked from commit 6d73002)

* [clang] Avoid reparsing VFS overlay files for module dep collector (llvm#158372)

This PR uses the new-ish `llvm::vfs::FileSystem::visit()` interface to
collect VFS overlay entries from an existing `FileSystem` instance
rather than parsing the VFS YAML file anew. This prevents duplicate
diagnostics as observed by `clang/test/VFS/broken-vfs-module-dep.c`.

(cherry picked from commit 4957c47)

* [clang] Don't fail `ExecuteCompilerInvocation()` due to caller errors (llvm#158695)

This PR changes the behavior of `clang::ExecuteCompilerInvocation()` so
that it only returns early when the `DiagnosticsEngine` emitted errors
**within** the function. Handling errors emitted before the function got
called is a responsibility of the caller. Necessary for llvm#158381.

(cherry picked from commit f33fb0d)

* [clang] Only set non-empty bypass to scan VFS (llvm#159605)

Normalizing an empty modules cache path results in an incorrect
non-empty path (the working directory). This PR conditionalizes more
code to avoid this. Tested downstream by swift/llvm-project and the
`DependencyScanningCAPITests.DependencyScanningFSCacheOutOfDate` unit
test.

(cherry picked from commit 5a339b0)

* Merge commit '0e35f56d40d3' from llvm.org/main into next

(cherry picked from commit 3efcc0f)

 Conflicts:
	clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

* [clang] NFCI: Clean up `CompilerInstance::create{File,Source}Manager()` (llvm#160748)

The `CompilerInstance::createSourceManager()` function currently accepts
the `FileManager` to be used. However, all clients call
`CompilerInstance::createFileManager()` prior to creating the
`SourceManager`, and it never makes sense to use a `FileManager` in the
`SourceManager` that's different from the rest of the compiler. Passing
the `FileManager` explicitly is redundant, error-prone, and deviates
from the style of other `CompilerInstance` initialization APIs.

This PR therefore removes the `FileManager` parameter from
`createSourceManager()` and also stops returning the `FileManager`
pointer from `createFileManager()`, since that was its primary use. Now,
`createSourceManager()` internally calls `getFileManager()` instead.

(cherry picked from commit b86ddae)

 Conflicts:
	clang/lib/Tooling/DependencyScanning/DependencyScannerImpl.cpp

* Merge commit '436861645247' from llvm.org/main into next

(cherry picked from commit 286ea7d)

 Conflicts:
	clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

* [clang] Pass VFS into `ASTUnit::LoadFromASTFile()` (llvm#159166)

This PR makes the `VFS` parameter to `ASTUnit::LoadFromASTFile()`
required and explicit, rather than silently defaulting to the real file
system. This makes it easy to correctly propagate the fully-configured
VFS and load any input files like the rest of the compiler does.

(cherry picked from commit cda542d)

* Fix a line missing when merging 30633f3

* [clang][deps] Fix a use-after-free from expanding response files (llvm#164676)

In 4368616 we accidentally moved uses of command-line args saved
into a bump pointer allocator during response file expansion out of
scope of the allocator. Also, the test that should have caught this (at
least with asan) was not working correctly because clang-scan-deps was
expanding response files itself during argument adjustment rather than
the underlying scanner library.

rdar://162720059
(cherry picked from commit 3e6f696)

---------

Co-authored-by: Jan Svoboda <jan_svoboda@apple.com>
Co-authored-by: James Y Knight <jyknight@google.com>
Co-authored-by: git apple-llvm automerger <am@git-apple-llvm>
Co-authored-by: Ben Langmuir <blangmuir@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang Clang issues not falling into any other category llvm:support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants